This might work for setiter_len, but setiter_iternext itself is not yet thread safe (also because of setting si->si_set to zero).

For several other iterations the approach is to keep the reference si->si_set , but use another attribute to signal exhaustion of the iterator. For example for itertools.cycle or the reversed operator.

Note: I tried creating a minimal example where concurrent iteration fails, but I have succeeded yet (the example does not crash, although I have not run thread sanitizer on it yet)

import unittest
from threading import Thread, Barrier


class TestSetIter(unittest.TestCase):
    def test_set_iter(self):
        """Test concurrent iteration over a set"""

        NUM_LOOPS = 10_000
        NUM_THREADS = 4
        

        for ii in range(NUM_LOOPS):
            if ii % 1000 ==0:
                print(f'test_set_iter {ii}')
            barrier = Barrier(NUM_THREADS)
            
            # make sure the underlying set is unique referenced by the iterator
            iterator = iter(set((1,2,))) 
            
            def worker():
                barrier.wait()
                while True:
                    iterator.__length_hint__()
                    try:
                        next(iterator)
                    except StopIteration:
                        break

                
            threads = [Thread(target=worker) for _ in range(NUM_THREADS)]
            for t in threads:
                t.start()
            for t in threads:
                t.join()
                
            assert iterator.__length_hint__()==0

if __name__ == "__main__":
    unittest.main()

Thank you. I think your points make a lot of sense, and I really appreciate the two links you shared—they helped me get a more complete picture of the iterator-related data race.
I’ll try to construct the case you mentioned under a TSan environment.
If it turns out to be appropriate, we can address it fully in this PR, that would be great. Of course, this will take some time.

Yes, we should fix this like we have fixed others and as Sam suggested only clear the associated set in non-free-threading builds. The current code is incorrect because it uses try incref which can fail spuriously if the set object is not marked to enable try incref.

I think we should follow the pattern that we use in other iterators: don't clear si->si_set when the iterator is exhausted in the free-threaded build.

That will keep other things simpler.

On the normal build the macro will expand to si->si_pos = i + 1;

This (and some other places) should also be atomic?

The value -1 written here could be overwritten by a concurrent thread (at line 1155). Which means that over exhaustion of the set iterator it is restored back to life. This does not lead to overflows or other issues (afaic), but is a bit odd behaviour.

I think in the normal build you still have to do si->si_set = NULL;, otherwise the si->si_set is decref'ed again in setiter_dealloc.

If we also set si->si_pos to -1 in the non-free threading build, then we can keep the code for the FT and non-FT the same I think. For the non-FT build we then set both si->si)set to zero and si->si_pos to -1 at exhaustion, but the code will be simpler.

Also the two code paths in setiter_iternext will be more similar then.

Thanks — using a flag makes the code clearer.

The test_iternext_concurrent_exhaust_race and test_length_hint_exhaust_race test slightly difference things, but I think they can be combined (to reduce pressure on the CI)

something like this work?

def worker():
    n = 0
    while True:
        if n & 1:
            it.__length_hint__()
        try:
            next(it)
        except StopIteration:
            break
        n += 1 

In the FT build so cannot be NULL. Remove the if (maybe replace with an assert).

Thanks. I applied the same change in both functions.

We can do the Py_DECREF(so); here and then remove the decref_so variable. (we are exhausted, so key is still NULL).